Skip to content

Extended fix for cl-like variables of CESM2 models#604

Merged
axel-lauer merged 2 commits into
masterfrom
fix_cesm2
Apr 28, 2020
Merged

Extended fix for cl-like variables of CESM2 models#604
axel-lauer merged 2 commits into
masterfrom
fix_cesm2

Conversation

@schlunma

@schlunma schlunma commented Apr 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR extends the fixes for cl, cli and clw for the CESM2 models (the original fix only flips the vertical coords, but not the data itself).

I also had to move the function var_name_constraint to a new module (iris_helpers) because of circular dependencies in esmvaltool.cmor._fixes.shared and esmvaltool.preprocessor.shared. I'm happy to rename that module if anybody knows a better name.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.

Related to #538 and #539.

@schlunma schlunma added the fix for dataset Related to dataset-specific fix files label Apr 1, 2020
@schlunma schlunma self-assigned this Apr 1, 2020
axel-lauer
axel-lauer approved these changes Apr 7, 2020
View reviewed changes

@axel-lauer axel-lauer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested these fixes for the CESM2 model family with variables "cli" and "clw". It works! Nice!

@schlunma

schlunma commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

Are the failing tests related to #189? @bouweandela @valeriupredoi

@valeriupredoi

valeriupredoi commented Apr 27, 2020

Copy link
Copy Markdown
Contributor

can you merge the latest master in this one pls @schlunma 🍺

@schlunma

schlunma commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

I did (one hour ago).

@valeriupredoi

valeriupredoi commented Apr 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

pytest-flake8=1.0.5 is being superflaky

@schlunma

schlunma commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

Ready to merge from my side

@bouweandela

bouweandela commented Apr 28, 2020

Copy link
Copy Markdown
Member

@mattiarighi Could you please test?

@schlunma Is there no other way to fix the formula terms than changing the file? Because this is really inefficient and I was hoping to get rid of that function at some point..

@axel-lauer

axel-lauer commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

@mattiarighi I'll rerun the tests and merge this PR in case the tests are successful.

@schlunma

schlunma commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

@schlunma Is there no other way to fix the formula terms than changing the file? Because this is really inefficient and I was hoping to get rid of that function at some point..

I invested quite some time in this and came to the following conclusion: It is possible without modifying the file, but the code necessary for this is considerably longer (it involves adding all coordinates, their bounds and manually adding the coordinate factory). Since the attribute formula_term is simply wrong, fixing this also feels like the "natural" way to fix this file.

@bouweandela

bouweandela commented Apr 28, 2020

Copy link
Copy Markdown
Member

it involves adding all coordinates, their bounds and manually adding the coordinate factory

Maybe there is some code in iris that you could copy/paste? It would be a lot faster, since file fixes copy the entire file before starting to do anything. If you don't have the time right now, maybe keep it as an idea for the future?

@schlunma

schlunma commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

Yes, I agree that this is slower. I will keep it in mind, but as I already said, it's way more complicated and also doesn't feel like the right fix.

@axel-lauer

axel-lauer commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

Tests were successful, PR is read to be merged...

@axel-lauer axel-lauer merged commit d817e69 into master Apr 28, 2020
@axel-lauer axel-lauer deleted the fix_cesm2 branch April 28, 2020 15:27
@schlunma schlunma mentioned this pull request Jun 23, 2020
Merged
10 tasks
@ledm ledm mentioned this pull request Nov 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bouweandela bouweandela bouweandela approved these changes
@axel-lauer axel-lauer axel-lauer approved these changes
@LisaBock LisaBock Awaiting requested review from LisaBock
@valeriupredoi valeriupredoi Awaiting requested review from valeriupredoi

Assignees

@schlunma schlunma

Labels

fix for dataset Related to dataset-specific fix files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@schlunma @valeriupredoi @bouweandela @axel-lauer